-
Notifications
You must be signed in to change notification settings - Fork 417
Only mark all mon updates complete if there are no blocked updates #3907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only mark all mon updates complete if there are no blocked updates #3907
Conversation
I've assigned @wpaulino as a reviewer! |
6bfe0bd
to
49ad15c
Compare
7759b4d
to
95c0696
Compare
95c0696
to
ab0dda7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3907 +/- ##
==========================================
- Coverage 88.82% 88.80% -0.02%
==========================================
Files 165 165
Lines 119075 119123 +48
Branches 119075 119123 +48
==========================================
+ Hits 105769 105790 +21
- Misses 10986 11003 +17
- Partials 2320 2330 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
@@ -8227,7 +8229,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ | |||
{ | |||
if chan.is_awaiting_monitor_update() { | |||
log_trace!(logger, "Channel is open and awaiting update, resuming it"); | |||
handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan); | |||
if chan.blocked_monitor_updates_pending() == 0 { | |||
handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we still want to call self.handle_monitor_update_completion_actions
so that we can actually unblock any monitor updates that are pending blocked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we don't do so in handle_new_monitor_update
's base case, which I was trying to match everywhere. A few tests fail if we change it there (with almost-harmless message ordering changes, so nothing major, but we'd have to fix that), but I'm not convinced its a bug - a channel should never block itself, and the unblocking of the next monitor update in a channel has to come in from a different channel or from an event, not from the channel itself. So that other channel, once it makes progress, should always unblock us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's what I figured, was just being overly-cautious in case we ever introduce such a monitor update that blocks the same channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think you're right that that change makes sense, but I don't think it makes sense as a backport and we should do it everywhere, not just in the new places, in a separate PR.
LGTM after squash |
ac695db
to
b47d7db
Compare
Squashed without further changes. |
✅ Added second reviewer: @joostjager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd ask someone with more background on the channel state machine as a 2nd reviewer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM logic-wise, nice catch. Had to re-read some code to refresh on the paths but it makes sense.
In `handle_new_monitor_update!`, we correctly check that the channel doesn't have any blocked monitor updates pending before calling `handle_monitor_update_completion!` (which calls `Channel::monitor_updating_restored`, which in turn assumes that all generated `ChannelMonitorUpdate`s, including blocked ones, have completed). We, however, did not do the same check at several other places where we called `handle_monitor_update_completion!`. Specifically, after a monitor update completes during reload (processed via a `BackgroundEvent` or when monitor update completes async, we didn't check if there were any blocked monitor updates before completing). Here we add the missing check, as well as an assertion in `Channel::monitor_updating_restored`.
b47d7db
to
418d59d
Compare
Pushed some tirivial cleanups: $ git diff-tree -U2 b47d7db4f1 418d59dff3
diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs
index 2a3f3b52ba..c8d408425f 100644
--- a/lightning/src/ln/chanmon_update_fail_tests.rs
+++ b/lightning/src/ln/chanmon_update_fail_tests.rs
@@ -3411,5 +3411,12 @@ fn test_inbound_reload_without_init_mon() {
}
-fn do_test_blocked_chan_preimage_release(completion_mode: u8) {
+#[derive(PartialEq, Eq)]
+enum BlockedUpdateComplMode {
+ Async,
+ AtReload,
+ Sync,
+}
+
+fn do_test_blocked_chan_preimage_release(completion_mode: BlockedUpdateComplMode) {
// Test that even if a channel's `ChannelMonitorUpdate` flow is blocked waiting on an event to
// be handled HTLC preimage `ChannelMonitorUpdate`s will still go out.
@@ -3459,5 +3466,5 @@ fn do_test_blocked_chan_preimage_release(completion_mode: u8) {
let as_htlc_fulfill_updates = get_htlc_update_msgs!(nodes[0], node_b_id);
- if completion_mode != 0 {
+ if completion_mode != BlockedUpdateComplMode::Sync {
// We use to incorrectly handle monitor update completion in cases where we completed a
// monitor update async or after reload. We test both based on the `completion_mode`.
@@ -3470,5 +3477,5 @@ fn do_test_blocked_chan_preimage_release(completion_mode: u8) {
assert!(get_monitor!(nodes[1], chan_id_2).get_stored_preimages().contains_key(&payment_hash_2));
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
- if completion_mode == 1 {
+ if completion_mode == BlockedUpdateComplMode::AtReload {
let node_ser = nodes[1].node.encode();
let chan_mon_0 = get_monitor!(nodes[1], chan_id_1).encode();
@@ -3487,5 +3494,5 @@ fn do_test_blocked_chan_preimage_release(completion_mode: u8) {
reconnect_nodes(a_b_reconnect);
reconnect_nodes(ReconnectArgs::new(&nodes[2], &nodes[1]));
- } else if completion_mode == 2 {
+ } else if completion_mode == BlockedUpdateComplMode::Async {
let (latest_update, _) = get_latest_mon_update_id(&nodes[1], chan_id_2);
nodes[1]
@@ -3499,6 +3506,7 @@ fn do_test_blocked_chan_preimage_release(completion_mode: u8) {
// update_fulfill_htlc + CS is held, even though the preimage is already on disk for the
// channel.
- // Note that in completion_mode 1 we completed the CS dance in `reconnect_nodes` above.
- if completion_mode != 1 {
+ // Note that when completing as a side effect of a reload we completed the CS dance in
+ // `reconnect_nodes` above.
+ if completion_mode != BlockedUpdateComplMode::AtReload {
nodes[1].node.handle_commitment_signed_batch_test(
node_a_id,
@@ -3547,7 +3555,7 @@ fn do_test_blocked_chan_preimage_release(completion_mode: u8) {
#[test]
fn test_blocked_chan_preimage_release() {
- do_test_blocked_chan_preimage_release(0);
- do_test_blocked_chan_preimage_release(1);
- do_test_blocked_chan_preimage_release(2);
+ do_test_blocked_chan_preimage_release(BlockedUpdateComplMode::AtReload);
+ do_test_blocked_chan_preimage_release(BlockedUpdateComplMode::Sync);
+ do_test_blocked_chan_preimage_release(BlockedUpdateComplMode::Async);
}
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index b9a0c01c6a..195fbc286e 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -8229,13 +8229,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
{
if chan.is_awaiting_monitor_update() {
- let should_resume = chan.blocked_monitor_updates_pending() == 0;
- let action_msg = if should_resume {
- "resuming it"
- } else {
- "leaving it blocked due to a blocked monitor update"
- };
- log_trace!(logger, "Channel is open and awaiting update, {action_msg}");
- if should_resume {
+ if chan.blocked_monitor_updates_pending() == 0 {
+ log_trace!(logger, "Channel is open and awaiting update, resuming it");
handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan);
+ } else {
+ log_trace!(logger, "Channel is open and awaiting update, leaving it blocked due to a blocked monitor update");
}
} else {
diff --git a/lightning/src/ln/quiescence_tests.rs b/lightning/src/ln/quiescence_tests.rs
index e6274f75f0..c2ab17e726 100644
--- a/lightning/src/ln/quiescence_tests.rs
+++ b/lightning/src/ln/quiescence_tests.rs
@@ -255,7 +255,7 @@ fn test_quiescence_waits_for_async_signer_and_monitor_update() {
// We have two updates pending:
{
- let chain_monitor = &nodes[0].chain_monitor;
+ let test_chain_mon = &nodes[0].chain_monitor;
let (_, latest_update) =
- chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
+ test_chain_mon.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
let chain_monitor = &nodes[0].chain_monitor.chain_monitor;
// One for the latest commitment transaction update from the last `revoke_and_ack`
@@ -263,9 +263,7 @@ fn test_quiescence_waits_for_async_signer_and_monitor_update() {
expect_payment_sent(&nodes[0], preimage, None, false, true);
- let chain_monitor = &nodes[0].chain_monitor;
let (_, new_latest_update) =
- chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
+ test_chain_mon.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
assert_eq!(new_latest_update, latest_update + 1);
- let chain_monitor = &nodes[0].chain_monitor.chain_monitor;
// One for the commitment secret update from the last `revoke_and_ack`
chain_monitor.channel_monitor_updated(chan_id, new_latest_update).unwrap(); |
Only diff since @wpaulino's ACK are the trivial changes above, so landing. |
In
handle_new_monitor_update!
, we correctly check that the channel doesn't have any blocked monitor updates pending before callinghandle_monitor_update_completion!
(which callsChannel::monitor_updating_restored
, which in turn assumes that all generatedChannelMonitorUpdate
s, including blocked ones, have completed).We, however, did not do the same check at several other places where we called
handle_monitor_update_completion!
. Specifically, after a monitor update completes during reload (processed via aBackgroundEvent
or when monitor update completes async, we didn't check if there were any blocked monitor updates before completing).Here we add the missing check, as well as an assertion in
Channel::monitor_updating_restored
.